Add WOLFSSL_STRICT_CIPHER_LIST for TLS 1.3-only cipher lists on a non-TLS1.3 ctx/ssl#10963
Add WOLFSSL_STRICT_CIPHER_LIST for TLS 1.3-only cipher lists on a non-TLS1.3 ctx/ssl#10963miyazakh wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds an opt-in build macro to make wolfSSL_{CTX,}_set_cipher_list() fail (instead of silently succeeding) when a TLS 1.3-only cipher list is applied to a context/SSL that cannot negotiate TLS 1.3, and adds tests/documentation to cover the new behavior.
Changes:
- Add
WOLFSSL_STRICT_CIPHER_LISTto make TLS 1.3-only cipher lists return failure on non-TLS1.3 ctx/ssl. - Document and register the new macro in internal build-options docs and known-macro list.
- Add a TLS 1.3 API test covering both default and strict behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/ssl.c |
Gates return value to fail under WOLFSSL_STRICT_CIPHER_LIST for TLS 1.3-only lists on non-TLS1.3 ctx/ssl. |
src/internal.c |
Documents the new build macro in the cipher suite selection options list. |
.wolfssl_known_macro_extras |
Registers WOLFSSL_STRICT_CIPHER_LIST as a known macro. |
tests/api/test_tls13.c |
Adds a new test exercising strict vs default behavior. |
tests/api/test_tls13.h |
Declares and registers the new test in the TLS 1.3 test group. |
Comments suppressed due to low confidence (1)
src/internal.c:1
- The text
wolfSSL_CTX/SSL_set_cipher_list()is ambiguous (it reads like a single function name). Consider updating to the exact API names (e.g.,wolfSSL_CTX_set_cipher_list()/wolfSSL_set_cipher_list()) for clarity and consistency with the rest of the build-options documentation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
retest this please |
3 similar comments
|
retest this please |
|
retest this please |
|
retest this please |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Medium] Test only exercises the CTX path, not the SSL path —
tests/api/test_tls13.c:2466-2489 - [Low] Test asserts return value but not that suites are actually left untouched —
tests/api/test_tls13.c:2480-2484 - [Low] WOLFSSL_STRICT_CIPHER_LIST can now fail ctx creation under WOLFSSL_SYS_CRYPTO_POLICY —
src/ssl.c:636-641
Review generated by Skoll via Claude/Codex
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| int test_tls13_cipher_list_no_tls13_ctx(void) |
There was a problem hiding this comment.
🟡 [Medium] Test only exercises the CTX path, not the SSL path
💡 SUGGEST test
The new macro gates the return value inside wolfSSL_parse_cipher_list(), which backs BOTH wolfSSL_CTX_set_cipher_list() (via ctx) and wolfSSL_set_cipher_list() (via ssl, src/ssl.c:4041). The PR body states the change and test cover both entry points, but the test only calls wolfSSL_CTX_set_cipher_list(). The ssl->version branch of the condition (ssl != NULL && !IsAtLeastTLSv1_3(ssl->version)) is never exercised, so a regression that broke only the SSL path would pass CI.
Suggestion: Add a parallel case that creates a WOLFSSL* from a TLS 1.2 ctx and calls wolfSSL_set_cipher_list(ssl, "TLS13-AES128-GCM-SHA256"), asserting WOLFSSL_FAILURE (strict) / WOLFSSL_SUCCESS (default), to cover the ssl->version branch.
| #ifdef WOLFSSL_STRICT_CIPHER_LIST | ||
| ExpectIntEQ(wolfSSL_CTX_set_cipher_list(ctx, "TLS13-AES128-GCM-SHA256"), | ||
| WOLFSSL_FAILURE); | ||
| #else |
There was a problem hiding this comment.
🔵 [Low] Test asserts return value but not that suites are actually left untouched
💡 SUGGEST test
The default-path comment claims the call succeeds "but leave ctx->suites untouched," yet the test only checks the WOLFSSL_SUCCESS return code. The distinguishing behavior (that the prior <=TLS1.2 suites remain and the TLS1.3-only list did NOT overwrite them) is not asserted. This is the exact silent-no-op behavior the PR is calling out as a footgun, so it would be valuable to pin it down.
Suggestion: Capture ctx->suites->suiteSz (or the suite bytes) before the call and assert they are unchanged after, confirming the TLS1.3-only list was actually ignored rather than merely returning success.
| @@ -3895,8 +3895,17 @@ static int wolfSSL_parse_cipher_list(WOLFSSL_CTX* ctx, WOLFSSL* ssl, | |||
| tls13Only = 1; | |||
There was a problem hiding this comment.
🔵 [Low] WOLFSSL_STRICT_CIPHER_LIST can now fail ctx creation under WOLFSSL_SYS_CRYPTO_POLICY
💡 SUGGEST question
Beyond the two public setters named in the PR, wolfSSL_parse_cipher_list() is also called during ctx construction when WOLFSSL_SYS_CRYPTO_POLICY is enabled. With WOLFSSL_STRICT_CIPHER_LIST defined, a system crypto policy that lists only TLS 1.3 suites applied to a <=TLS1.2 method would now make wolfSSL_parse_cipher_list() return WOLFSSL_FAILURE, causing wolfSSL_CTX_new_ex to free the ctx and return NULL where it previously succeeded. This is a plausibly-intended consequence of opting into strict behavior (fail loudly), but it is a broader blast radius than the two setters described in the PR body and is not covered by tests. Worth confirming this interaction is intended and documenting it.
Recommendation: Confirm the WOLFSSL_SYS_CRYPTO_POLICY + WOLFSSL_STRICT_CIPHER_LIST interaction (ctx creation can now fail) is intended, and note it in the macro documentation.
Summary(f3228)
wolfSSL_CTX_set_cipher_list()/wolfSSL_set_cipher_list()silentlyreturn
WOLFSSL_SUCCESSwithout touching the configured suites whengiven a cipher list that names only TLS 1.3 suites, but the
ctx/sslcannot negotiate TLS 1.3 (
OPENSSL_EXTRAbuilds only, viawolfSSL_parse_cipher_list()insrc/ssl.c).SSL_CTX_set_ciphersuites()compatibility shim,but a caller that treats the non-failing return as confirmation that
the restriction took effect will keep negotiating whatever
<= TLS 1.2suites were already configured (default or otherwise).
WOLFSSL_STRICT_CIPHER_LISTbuild macro. When defined,the function returns
WOLFSSL_FAILUREinstead of silently ignoringthe list. Off by default, so existing OpenSSL-compat behavior is
unchanged unless a user explicitly opts in.
Changes
src/ssl.c: gate the return value inwolfSSL_parse_cipher_list()behind
WOLFSSL_STRICT_CIPHER_LIST.src/internal.c: document the new macro in the build-options headercomment (Cipher Suite Selection section).
.wolfssl_known_macro_extras: registerWOLFSSL_STRICT_CIPHER_LIST.tests/api/test_tls13.c,tests/api/test_tls13.h: addtest_tls13_cipher_list_no_tls13_ctx, covering both the default(
WOLFSSL_SUCCESS) and strict (WOLFSSL_FAILURE) behavior.Testing
./configure --enable-opensslextra --enable-tls13 --enable-debug && make— builds clean../tests/unit.test(default,WOLFSSL_STRICT_CIPHER_LISTundefined):
Failed/Skipped/Passed/All: 0/314/1726/2040../tests/unit.test(CPPFLAGS="-DWOLFSSL_STRICT_CIPHER_LIST"):Failed/Skipped/Passed/All: 0/314/1726/2040.test_tls13_cipher_list_no_tls13_ctxpasses in bothconfigurations, exercising
wolfSSL_CTX_set_cipher_list(ctx, "TLS13-AES128-GCM-SHA256")on awolfTLSv1_2_client_method()context.
Checklist